timeLimitsManager: Hold inhibitor for sleep to allow saving screen time data

This avoids the race between systemd emitting the `prepare-for-sleep`
signal, gnome-shell then starting to write the screen time data to disk,
and systemd suspending the hardware.

The race isn’t so much of an issue if the suspend succeeds (if
gnome-shell loses, the data will still get written out when the machine
resumes), but it’s slightly problematic if the machine loses power while
suspended, as that means the latest screen time data is lost.

Includes significant suggestions from Florian Müllner.

Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Part-of: <https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/3643>

diff --git a/js/misc/timeLimitsManager.js b/js/misc/timeLimitsManager.js
index 6be0e456ae8cfac5ea865f3b0e2b8a8164e30659..776076464dd87df32d43cd018658141306005d94 100644 (file)
--- a/js/misc/timeLimitsManager.js
+++ b/js/misc/timeLimitsManager.js
@@ -169,6 +169,7 @@ export const TimeLimitsManager = GObject.registerClass({
         this._stateTransitions = [];
         this._cancellable = null;
         this._loginManager = null;
+        this._inhibitor = null;
         this._loginUser = null;
         this._lastStateChangeTimeSecs = 0;
         this._timerId = 0;
@@ -257,12 +258,16 @@ export const TimeLimitsManager = GObject.registerClass({
             this._timeChangeId = 0;
         }
 
-        // Start listening for notifications to the user’s state.
+        // Start listening for notifications to the user’s state. Listening to
+        // the prepare-for-sleep signal requires taking a delay inhibitor to
+        // avoid races.
         this._loginManager = this._loginManagerFactory.new();
+        await this._ensureInhibitor();
         this._loginManager.connectObject(
             'prepare-for-sleep',
-            () => this._updateUserState(true).catch(
-                e => console.warn(`Failed to update user state: ${e.message}`)),
+            (unused, preparingForSleep) => {
+                this._onPrepareForSleep(preparingForSleep).catch(logError);
+            },
             this);
 
         this._loginUser = await this._loginUserFactory.newAsync();
@@ -290,6 +295,8 @@ export const TimeLimitsManager = GObject.registerClass({
         this._loginUser?.disconnectObject(this);
         this._loginUser = null;
 
+        this._releaseInhibitor();
+
         this._loginManager?.disconnectObject(this);
         this._loginManager = null;
 
@@ -322,6 +329,40 @@ export const TimeLimitsManager = GObject.registerClass({
         this._cancellable = null;
     }
 
+    async _ensureInhibitor() {
+        if (this._inhibitor)
+            return;
+
+        try {
+            this._inhibitor = await this._loginManager.inhibit(
+                _('GNOME needs to save screen time data'), this._cancellable);
+        } catch (e) {
+            if (!e.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED))
+                console.warn('Failed to inhibit suspend: %s'.format(e.message));
+        }
+    }
+
+    _releaseInhibitor() {
+        this._inhibitor?.close(null);
+        this._inhibitor = null;
+    }
+
+    async _onPrepareForSleep(preparingForSleep) {
+        // Just come back from sleep, so take another inhibitor.
+        if (!preparingForSleep)
+            this._ensureInhibitor();
+
+        try {
+            await this._updateUserState(true);
+        } catch (e) {
+            console.warn(`Failed to update user state: ${e.message}`);
+        }
+
+        // Release the inhibitor if we’re preparing to sleep.
+        if (preparingForSleep)
+            this._releaseInhibitor();
+    }
+
     /** Shut down the state machine and write out the state file. */
     async shutdown() {
         await this._stopStateMachine();

diff --git a/tests/unit/timeLimitsManager.js b/tests/unit/timeLimitsManager.js
index 578ed1c628dc79e709636975940eba0d360c8cdb..3eba0aa7c6de195aa5983e4a629bda9b0ab1b59d 100644 (file)
--- a/tests/unit/timeLimitsManager.js
+++ b/tests/unit/timeLimitsManager.js
@@ -91,6 +91,12 @@ class TestHarness {
         // And a mock D-Bus proxy for logind.
         const harness = this;
 
+        class MockInhibitor {
+            close(unusedCancellable) {
+                // No-op for mock purposes
+            }
+        }
+
         class MockLoginManager {
             connectObject(signalName, callback, unusedObject) {
                 if (signalName === 'prepare-for-sleep') {
@@ -107,6 +113,12 @@ class TestHarness {
                 harness._loginManagerPrepareForSleepCallback = null;
             }
 
+            /* eslint-disable-next-line require-await */
+            async inhibit(unusedReason, unusedCancellable) {
+                // Basically a no-op for mock purposes
+                return new MockInhibitor();
+            }
+
             get preparingForSleep() {
                 return harness._currentPreparingForSleepState;
             }
@@ -578,8 +590,10 @@ class TestHarness {
         case 'preparing-for-sleep-state-change':
             this._currentPreparingForSleepState = event.newPreparingForSleepState;
 
-            if (this._loginManagerPrepareForSleepCallback)
-                this._loginManagerPrepareForSleepCallback(this._currentPreparingForSleepState);
+            if (this._loginManagerPrepareForSleepCallback) {
+                this._loginManagerPrepareForSleepCallback(
+                    this._mockLoginManager, this._currentPreparingForSleepState);
+            }
             break;
         case 'login-user-state-change':
             this._currentUserState = event.newUserState;